fix(valid-repository-directory): use repository root for more accurate linting #498
Conversation
|
@JoshuaKGoldberg I left this PR in "draft" mode because I wanted to ask if you knew how to test this as an actual ESLint rule? There are the unit tests but I would love to test this in a real repo if possible. Any clue? Or can we publish a beta or something to npm and I'll test it that way? Once it's better tested I can publish. |
|
@JoshuaKGoldberg OH, shoot: The tests are failing because I published @altano/repository-tools as an ESM-only package, and this lint rule has a CJS build which fails to run b/c of the error: We have options to proceed here:
Is 1 the only viable option? Thoughts? |
|
Yeah I think we're stuck with 1 😞. This package's version support range includes Node versions older than the require-sync-ESM-in-CJS bridge. And a lot of the ESLint ecosystem is still running on CJS. Tangentially related, a thought: |
|
Alright, yeah, I don't see any other way forward, so I'll publish a dual package. w.r.t. package surface area: I was planning on adding more utility functions to the "repository-tools" package, but only ones that are thin wrappers around the CLI commands. For example, the |
|
@JoshuaKGoldberg I investigated the best way to break a monolithic package up. Packages like lodash seem to have gone through multiple methods (including having separate packages for each method) and landed on the approach of having one package, but allowing subpath exports into the individual files (this is documented here). Then you get the best of all worlds: it's tree-shaking friendly, but shared code won't be duplicated across sub-packages. So, in addition to supporting ESM/CJS, I'm adding sub-path exports so you can do this: import { findRootSync } from "@altano/repository-tools/findRootSync";I also added unit tests to actually exercise the sub-path exports and make sure they work. Does this all sound good to you? To reiterate: I'm down for whatever, so feel free to suggest an alternative. |
|
Cool, that sounds good to me! I'm not super picky right now. Even if we somehow get it horribly wrong now, as you said, the package is pretty small. We can always tweak it later. 🙂 |
altano
force-pushed
the
valid-repository-directory
branch
5 times, most recently
from
4fda680 to
02018de
Compare
There was a problem hiding this comment.
OK! This is phenomenal, awesome job @altano! I tried this out in a few repositories locally. I couldn't break it at all. Nice! 👏
I just have little nitpicky questions that don't really change much. I can go ahead and apply them in a couple days.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #498 +/- ##
==========================================
+ Coverage 98.79% 98.88% +0.08%
==========================================
Files 17 17
Lines 995 1074 +79
Branches 93 100 +7
==========================================
+ Hits 983 1062 +79
Misses 12 12 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
altano
force-pushed
the
valid-repository-directory
branch
from
e480176 to
9962ab8
Compare
|
The compliance action failed with this message: 🤷🏽♀️ |
|
Thanks for the review. All comments resolved or replied to. Back in your court! |
There was a problem hiding this comment.
OK! Thanks for all your work on this @altano, it's looking great!
|
🎉 This is included in version v0.15.3 🎉 The release is available on: Cheers! 📦🚀 |
PR Checklist
status: accepting prsOverview
Commits
There are two commits:
1. "settings" - these are just ignores I'm adding to keep my local development environment out of this project (e.g. I use devbox to manage my tool chain, so I'm ignoring those files from the repo)2. "fix ..." - the actual lint rule fixes
@altano/repository-tools Dependency
The code changes here aren't that big because we're leaning on my @altano/repository-tools library to do the hardest part: discovering the root of the repository.
Notes about this package:
SCMs supported: git / mercurial / sapling / subversion
Performance: the library can find a git repository's root in ~3ms, and the worst (slowest) repository type (Mercurial) takes ~148ms. I think this is totally acceptable for package.json linting:
This repository is well tested. As part of the CI flow it has unit tests that create real repositories for each supported type and fully exercise the code.
If you don't want to depend on this library, we can pull in the very small amount of code directly into this repo, but you'd be losing out on the really good CI testing.
Rule Fix
When in a supported repository, the logic for the lint rule is simple:
"directory"field should be that path"directory"in package.json is"packages/packageA"When a repository root cannot be found, there is a fallback:
"directory"value appears at the end of the package.json directory path. Since we don't know the root of the repository, we can't tell how much of the path has to be present. This can be inaccurate. For example, if package.json is at '/a/b/c/package.json':"directory"is either "/a/b/c" or "b/c" or "c" (these all validate)